POC : apply modifications on a case (to be used by monitor-core)#771
Open
FranckLecuyer wants to merge 7 commits intomainfrom
Open
POC : apply modifications on a case (to be used by monitor-core)#771FranckLecuyer wants to merge 7 commits intomainfrom
FranckLecuyer wants to merge 7 commits intomainfrom
Conversation
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
Signed-off-by: Franck LECUYER <franck.lecuyer@rte-france.com>
📝 WalkthroughWalkthroughA new REST endpoint and service layer enable applying composite network modifications to a case on a remote case server. The implementation handles network import, modification application with error handling, report aggregation, network persistence, and result notification via RabbitMQ messaging. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as NetworkModificationOnCaseController
participant Service as NetworkModificationOnCaseService
participant NetConv as NetworkConversionService
participant CaseServer as Case Server
participant ModService as Modification Service
participant ReportService
participant NotifService as Notification Service
participant RabbitMQ
Client->>Controller: POST /{caseUuid}/network-composite-modifications
Controller->>Service: applyNetworkCompositeModificationsOnCase(caseUuid, executionUuid, uuids)
Service->>NetConv: createNetwork(caseUuid, reportNode)
NetConv->>CaseServer: Retrieve case data
CaseServer-->>NetConv: Case data (XIIDM)
NetConv->>NetConv: Import network via PowSyBL Importer
NetConv-->>Service: Network object
Service->>ModService: Fetch ModificationInfos for each UUID
ModService-->>Service: List of ModificationInfos
loop For each activated modification
Service->>Service: Convert ModificationInfos to AbstractModification
Service->>Service: Run modification check
Service->>Service: Apply modification to network
Service->>ReportService: Update report node
alt Modification fails
Service->>Service: Log exception, continue iteration
end
end
Service->>ReportService: Finalize and send execution report
ReportService-->>Service: reportUuid
Service->>CaseServer: Persist modified network (XIIDM format)
CaseServer-->>Service: resultCaseUuid
Service->>Service: Create CaseResultInfos with status and UUIDs
Service->>NotifService: sendMessage(CaseResultInfos, publishCaseResult-out-0)
NotifService->>RabbitMQ: Publish to modifications.case.result
Service-->>Controller: Return
Controller-->>Client: HTTP 200 OK
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
pom.xml (1)
71-71: Add a comment explaining why this exclusion is needed.The test classpath exclusion for
powsybl-iidm-implis added without explanation. Line 70 excludespowsybl-config-classicfor similar reasons, but future maintainers would benefit from understanding why this exclusion is necessary (e.g., conflict withpowsybl-network-store-iidm-implduring tests).📝 Suggested documentation
<classpathDependencyExcludes> <classpathDependencyExclude>com.powsybl:powsybl-config-classic</classpathDependencyExclude> + <!-- Exclude to avoid conflicts with powsybl-network-store-iidm-impl during tests --> <classpathDependencyExclude>com.powsybl:powsybl-iidm-impl</classpathDependencyExclude> </classpathDependencyExcludes>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` at line 71, Add an inline XML comment above the <classpathDependencyExclude>com.powsybl:powsybl-iidm-impl</classpathDependencyExclude> entry explaining why this exclusion is required for tests (e.g., it conflicts with or duplicates classes from com.powsybl:powsybl-network-store-iidm-impl or causes resolution/implementation collisions), mirroring the rationale used for the adjacent com.powsybl:powsybl-config-classic exclusion so future maintainers understand the purpose.src/main/java/org/gridsuite/modification/server/service/CaseResultInfos.java (1)
16-38: Consider using a Java record for this immutable DTO.Since Java 17+ is typically used, a record provides a more concise and idiomatic way to define an immutable data carrier. Records automatically provide
equals(),hashCode(), andtoString(), which are useful for logging and debugging message payloads.♻️ Suggested refactor using record
-import lombok.Getter; - -import java.util.UUID; - -/** - * `@author` Franck Lecuyer <franck.lecuyer at rte-france.com> - */ -@Getter -public class CaseResultInfos { - private final UUID caseResultUuid; - - private final UUID executionUuid; - - private final UUID reportUuid; - - private final UUID resultUuid; - - private final String stepType; - - private final String status; - - public CaseResultInfos(UUID caseResultUuid, UUID executionUuid, UUID reportUuid, UUID resultUuid, String stepType, String status) { - this.caseResultUuid = caseResultUuid; - this.executionUuid = executionUuid; - this.reportUuid = reportUuid; - this.resultUuid = resultUuid; - this.stepType = stepType; - this.status = status; - } -} +import java.util.UUID; + +/** + * `@author` Franck Lecuyer <franck.lecuyer at rte-france.com> + */ +public record CaseResultInfos( + UUID caseResultUuid, + UUID executionUuid, + UUID reportUuid, + UUID resultUuid, + String stepType, + String status +) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/service/CaseResultInfos.java` around lines 16 - 38, Replace the immutable DTO class CaseResultInfos with a Java record to reduce boilerplate: remove the class declaration, final fields and explicit constructor and declare a record named CaseResultInfos with components (UUID caseResultUuid, UUID executionUuid, UUID reportUuid, UUID resultUuid, String stepType, String status) so you get autogenerated accessors, equals, hashCode and toString; ensure any existing usages calling getters (e.g., getCaseResultUuid()) are updated to use the record component accessors (caseResultUuid()) or left as-is if your code uses Lombok-style `@Getter` behavior.src/main/java/org/gridsuite/modification/server/NetworkModificationOnCaseController.java (2)
39-47: Clarify the asynchronous nature of this endpoint.The endpoint returns
200 OKimmediately, but the actual modifications appear to be applied asynchronously with results published via RabbitMQ. Consider:
- Returning
202 Acceptedinstead of200 OKto indicate the request has been accepted for processing but not yet completed- Adding appropriate
@ApiResponseannotations for error cases (400, 404, 500)- Documenting that clients should subscribe to the
modifications.case.resulttopic for completion notifications♻️ Suggested improvements
`@PostMapping`(value = "/{caseUuid}/network-composite-modifications", produces = MediaType.APPLICATION_JSON_VALUE) `@Operation`(summary = "Apply a list of composite modifications on a case") -@ApiResponse(responseCode = "200", description = "Composite modifications applied on case") +@ApiResponse(responseCode = "202", description = "Request accepted, modifications being applied asynchronously") +@ApiResponse(responseCode = "400", description = "Invalid request parameters") +@ApiResponse(responseCode = "404", description = "Case or composite modification not found") +@ApiResponse(responseCode = "500", description = "Internal server error") public ResponseEntity<Void> applyNetworkCompositeModificationsOnCase(...) { networkModificationOnCaseService.applyNetworkCompositeModificationsOnCase(caseUuid, executionUuid, compositeModificationUuids); - return ResponseEntity.ok().build(); + return ResponseEntity.accepted().build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/NetworkModificationOnCaseController.java` around lines 39 - 47, The endpoint applyNetworkCompositeModificationsOnCase in NetworkModificationOnCaseController is asynchronous but currently returns 200 OK and lacks error responses and documentation; change the controller to return 202 Accepted (ResponseEntity.accepted()) instead of ResponseEntity.ok(), add `@ApiResponse` annotations for 400, 404 and 500 on the method signature, and update the Operation/JavaDoc to explicitly state processing is asynchronous and clients should subscribe to the modifications.case.result RabbitMQ topic for completion notifications so callers know to listen for results.
44-44: Consider accepting composite modification UUIDs in the request body.Passing a list of UUIDs as a query parameter (
@RequestParam("uuids") List<UUID>) can lead to long URLs and potential issues with URL length limits when many modifications are specified. REST conventions typically favor request bodies for POST operations with complex or variable-length data.♻️ Alternative design using request body
-public ResponseEntity<Void> applyNetworkCompositeModificationsOnCase(`@Parameter`(description = "Case UUID") `@PathVariable`("caseUuid") UUID caseUuid, - `@Parameter`(description = "Execution UUID") `@RequestParam`(name = "executionUuid", required = false) UUID executionUuid, - `@Parameter`(description = "Composite modifications uuids list") `@RequestParam`("uuids") List<UUID> compositeModificationUuids) { +public ResponseEntity<Void> applyNetworkCompositeModificationsOnCase(`@Parameter`(description = "Case UUID") `@PathVariable`("caseUuid") UUID caseUuid, + `@Parameter`(description = "Execution UUID") `@RequestParam`(name = "executionUuid", required = false) UUID executionUuid, + `@Parameter`(description = "Composite modifications uuids list") `@RequestBody` List<UUID> compositeModificationUuids) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/NetworkModificationOnCaseController.java` at line 44, The controller method in NetworkModificationOnCaseController that currently accepts compositeModificationUuids via `@RequestParam`("uuids") List<UUID> should be changed to accept the UUID list in the request body to avoid long URLs; replace the `@RequestParam` parameter with an `@RequestBody` annotated payload (either a simple List<UUID> or a small DTO like CompositeModificationUuidsRequest { List<UUID> uuids; }) and update the method signature accordingly, remove/adjust the `@Parameter/`@RequestParam annotations, and update any OpenAPI/Swagger annotations and callers/tests to POST the UUID list in the JSON body instead of as query params.src/main/java/org/gridsuite/modification/server/service/NetworkConversionService.java (1)
35-38: Consider configuring timeouts on the RestTemplate.The
RestTemplateis created without explicit connection and read timeouts. If the case-server is slow or unresponsive, requests could hang indefinitely, potentially exhausting threads.♻️ Suggested timeout configuration
+import java.time.Duration; + public NetworkConversionService(`@Value`("${powsybl.services.case-server.base-uri:http://case-server/}") String caseServerBaseUri, RestTemplateBuilder restTemplateBuilder) { - this.caseServerRest = restTemplateBuilder.rootUri(caseServerBaseUri).build(); + this.caseServerRest = restTemplateBuilder + .rootUri(caseServerBaseUri) + .connectTimeout(Duration.ofSeconds(30)) + .readTimeout(Duration.ofMinutes(5)) + .build(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/service/NetworkConversionService.java` around lines 35 - 38, The RestTemplate created in NetworkConversionService constructor lacks timeouts; update the constructor to configure connect and read timeouts on the RestTemplateBuilder (e.g., restTemplateBuilder.setConnectTimeout(Duration.ofSeconds(5)).setReadTimeout(Duration.ofSeconds(30)).rootUri(caseServerBaseUri).build()), import java.time.Duration, and assign the result to the caseServerRest field so requests to the case-server cannot hang indefinitely.src/main/java/org/gridsuite/modification/server/service/NotificationService.java (1)
39-42: Consider adding a dedicated method for case result notifications.Exposing the generic
sendMessagemethod publicly expands the API surface and allows callers to send to arbitrary binding names. The existing pattern (e.g.,emitBuildResultMessage,emitCancelBuildMessage) wraps the low-levelsendMessagewith type-safe, purpose-specific methods.For consistency and encapsulation, consider adding a method like
emitCaseResultMessage(CaseResultInfos payload)that internally constructs the message and callssendMessagewith the correct binding name.♻️ Suggested approach
+ public void emitCaseResultMessage(`@NonNull` CaseResultInfos payload) { + Message<CaseResultInfos> message = MessageBuilder.withPayload(payload).build(); + sendMessage(message, "publishCaseResult-out-0"); + } + - public void sendMessage(Message<? extends Object> message, String bindingName) { + private void sendMessage(Message<? extends Object> message, String bindingName) { OUTPUT_MESSAGE_LOGGER.debug("Sending message : {}", message); publisher.send(bindingName, message); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/service/NotificationService.java` around lines 39 - 42, Add a dedicated, type-safe method emitCaseResultMessage(CaseResultInfos payload) in NotificationService that constructs a Message from the payload and calls the existing sendMessage(...) with the correct binding name (following the pattern used by emitBuildResultMessage and emitCancelBuildMessage); then restrict the generic sendMessage(...) visibility to private (or package-private) so callers cannot send to arbitrary bindings and the public API surface is limited to purpose-specific emitXxx methods, referencing publisher.send(...) inside sendMessage for actual dispatch.src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java (4)
82-96: Remove shadowing parameterfilterService.The method parameter
filterServiceshadows the instance field and is always invoked withthis.filterService(line 154). This is confusing and error-prone. Use the instance field directly.♻️ Suggested refactor
- private void applyModifications(Network network, List<ModificationInfos> modificationsInfos, ReportNode reportNode, FilterService filterService) { + private void applyModifications(Network network, List<ModificationInfos> modificationsInfos, ReportNode reportNode) { modificationsInfos.stream() .filter(ModificationInfos::getActivated) .forEach(modificationInfos -> { try { AbstractModification modification = modificationInfos.toModification(); modification.check(network); - modification.initApplicationContext(filterService, null); + modification.initApplicationContext(this.filterService, null); modification.apply(network, reportNode); } catch (Exception e) { // For now, we just log the error, and we continue to apply the following modifications handleException(modificationInfos.getErrorType(), e); } }); }Update the call site at line 154:
- applyModifications(network, modifications, rootReport, filterService); + applyModifications(network, modifications, rootReport);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java` around lines 82 - 96, The applyModifications method currently shadows the instance field by taking a parameter named filterService; remove the filterService parameter from the applyModifications signature, update the implementation to reference the instance field (this.filterService) directly (e.g., when calling modification.initApplicationContext), and update all call sites that pass this.filterService to call the new no-parameter applyModifications; ensure imports/signatures and any tests are updated so compilation succeeds and behavior remains unchanged.
57-57: Consider makingcaseExportFormata static constant.This field is effectively a constant since it's initialized inline and never modified. Declaring it as
private static finalclarifies intent and avoids per-instance storage.♻️ Suggested change
- private final String caseExportFormat = "XIIDM"; + private static final String CASE_EXPORT_FORMAT = "XIIDM";Then update usages at lines 112 and 115 to use
CASE_EXPORT_FORMAT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java` at line 57, Change the instance field caseExportFormat into a class constant by replacing "private final String caseExportFormat = \"XIIDM\";" with "private static final String CASE_EXPORT_FORMAT = \"XIIDM\";" in NetworkModificationOnCaseService, and update all internal references (the places currently using caseExportFormat) to use CASE_EXPORT_FORMAT instead; remove the old instance field so the value is no longer stored per-instance.
98-108: No timeout or error handling for case server REST calls.The
RestTemplatecalls to the case server (lines 107, and implicitly throughnetworkConversionService) have no configured timeouts. Long-running or hung requests could block indefinitely. Consider configuring connection and read timeouts on theRestTemplateBuilder.Example timeout configuration
this.restTemplate = restTemplateBuilder .rootUri(caseServerBaseUri) .setConnectTimeout(Duration.ofSeconds(10)) .setReadTimeout(Duration.ofSeconds(60)) .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java` around lines 98 - 108, The save(Resource resource) method (and other places using restTemplate, e.g., calls via networkConversionService) lack timeouts and error handling; fix by configuring the RestTemplate via RestTemplateBuilder when it is instantiated: set a rootUri, setConnectTimeout and setReadTimeout (e.g., 10s/60s) and assign that configured instance to the restTemplate field, and add try/catch around restTemplate.postForObject in save(Resource) to catch ResourceAccessException/HttpStatusCodeException (log context including uri and resource id) and translate or rethrow a meaningful application exception so callers don’t block indefinitely or fail without diagnostics.
156-158: Consider usingReportMode.REPLACEfor a newly created report.Since
reportUuidis freshly generated, there's no existing report to append to. UsingREPLACEmore accurately reflects the intent of creating a complete, standalone report. This aligns with test patterns whereReportMode.REPLACEis used for initial report creation.♻️ Suggested change
// send report to report server reportUuid = UUID.randomUUID(); - reportService.sendReport(reportUuid, rootReport, ReportMode.APPEND); + reportService.sendReport(reportUuid, rootReport, ReportMode.REPLACE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java` around lines 156 - 158, The code generates a fresh reportUuid then calls reportService.sendReport(reportUuid, rootReport, ReportMode.APPEND) which appends to an existing report; change the mode to ReportMode.REPLACE so the newly created reportUuid is saved as a standalone report (update the call in NetworkModificationOnCaseService where reportUuid and reportService.sendReport are used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java`:
- Around line 110-122: In save(Network network) guard against
memDataSource.listNames(".*") returning empty: check if listNames.isEmpty() and
throw an IOException (or a specific checked exception) with a clear message
before using toArray()[0]; then replace the fragile toArray()[0] access with a
safe retrieval (e.g., listNames.iterator().next() or
listNames.stream().findFirst().get()) and continue creating the
ByteArrayResource with caseFileName and delegate to save(...). Ensure you
reference the memDataSource, listNames, caseExportFormat, ByteArrayResource and
the save(...) call when making the change.
- Around line 162-163: The catch block in NetworkModificationOnCaseService that
sets status = "FAILED" currently swallows the exception; update that catch to
log the exception (including stacktrace) using the class logger (e.g.,
logger.error("Failed to process network modification for case {}: {}",
caseIdOrContext, e.getMessage(), e)) so the error and context are recorded;
ensure you reference the existing logger field in
NetworkModificationOnCaseService and include any relevant identifiers (case
id/tenant id) available in the method to aid troubleshooting.
---
Nitpick comments:
In `@pom.xml`:
- Line 71: Add an inline XML comment above the
<classpathDependencyExclude>com.powsybl:powsybl-iidm-impl</classpathDependencyExclude>
entry explaining why this exclusion is required for tests (e.g., it conflicts
with or duplicates classes from com.powsybl:powsybl-network-store-iidm-impl or
causes resolution/implementation collisions), mirroring the rationale used for
the adjacent com.powsybl:powsybl-config-classic exclusion so future maintainers
understand the purpose.
In
`@src/main/java/org/gridsuite/modification/server/NetworkModificationOnCaseController.java`:
- Around line 39-47: The endpoint applyNetworkCompositeModificationsOnCase in
NetworkModificationOnCaseController is asynchronous but currently returns 200 OK
and lacks error responses and documentation; change the controller to return 202
Accepted (ResponseEntity.accepted()) instead of ResponseEntity.ok(), add
`@ApiResponse` annotations for 400, 404 and 500 on the method signature, and
update the Operation/JavaDoc to explicitly state processing is asynchronous and
clients should subscribe to the modifications.case.result RabbitMQ topic for
completion notifications so callers know to listen for results.
- Line 44: The controller method in NetworkModificationOnCaseController that
currently accepts compositeModificationUuids via `@RequestParam`("uuids")
List<UUID> should be changed to accept the UUID list in the request body to
avoid long URLs; replace the `@RequestParam` parameter with an `@RequestBody`
annotated payload (either a simple List<UUID> or a small DTO like
CompositeModificationUuidsRequest { List<UUID> uuids; }) and update the method
signature accordingly, remove/adjust the `@Parameter/`@RequestParam annotations,
and update any OpenAPI/Swagger annotations and callers/tests to POST the UUID
list in the JSON body instead of as query params.
In
`@src/main/java/org/gridsuite/modification/server/service/CaseResultInfos.java`:
- Around line 16-38: Replace the immutable DTO class CaseResultInfos with a Java
record to reduce boilerplate: remove the class declaration, final fields and
explicit constructor and declare a record named CaseResultInfos with components
(UUID caseResultUuid, UUID executionUuid, UUID reportUuid, UUID resultUuid,
String stepType, String status) so you get autogenerated accessors, equals,
hashCode and toString; ensure any existing usages calling getters (e.g.,
getCaseResultUuid()) are updated to use the record component accessors
(caseResultUuid()) or left as-is if your code uses Lombok-style `@Getter`
behavior.
In
`@src/main/java/org/gridsuite/modification/server/service/NetworkConversionService.java`:
- Around line 35-38: The RestTemplate created in NetworkConversionService
constructor lacks timeouts; update the constructor to configure connect and read
timeouts on the RestTemplateBuilder (e.g.,
restTemplateBuilder.setConnectTimeout(Duration.ofSeconds(5)).setReadTimeout(Duration.ofSeconds(30)).rootUri(caseServerBaseUri).build()),
import java.time.Duration, and assign the result to the caseServerRest field so
requests to the case-server cannot hang indefinitely.
In
`@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java`:
- Around line 82-96: The applyModifications method currently shadows the
instance field by taking a parameter named filterService; remove the
filterService parameter from the applyModifications signature, update the
implementation to reference the instance field (this.filterService) directly
(e.g., when calling modification.initApplicationContext), and update all call
sites that pass this.filterService to call the new no-parameter
applyModifications; ensure imports/signatures and any tests are updated so
compilation succeeds and behavior remains unchanged.
- Line 57: Change the instance field caseExportFormat into a class constant by
replacing "private final String caseExportFormat = \"XIIDM\";" with "private
static final String CASE_EXPORT_FORMAT = \"XIIDM\";" in
NetworkModificationOnCaseService, and update all internal references (the places
currently using caseExportFormat) to use CASE_EXPORT_FORMAT instead; remove the
old instance field so the value is no longer stored per-instance.
- Around line 98-108: The save(Resource resource) method (and other places using
restTemplate, e.g., calls via networkConversionService) lack timeouts and error
handling; fix by configuring the RestTemplate via RestTemplateBuilder when it is
instantiated: set a rootUri, setConnectTimeout and setReadTimeout (e.g.,
10s/60s) and assign that configured instance to the restTemplate field, and add
try/catch around restTemplate.postForObject in save(Resource) to catch
ResourceAccessException/HttpStatusCodeException (log context including uri and
resource id) and translate or rethrow a meaningful application exception so
callers don’t block indefinitely or fail without diagnostics.
- Around line 156-158: The code generates a fresh reportUuid then calls
reportService.sendReport(reportUuid, rootReport, ReportMode.APPEND) which
appends to an existing report; change the mode to ReportMode.REPLACE so the
newly created reportUuid is saved as a standalone report (update the call in
NetworkModificationOnCaseService where reportUuid and reportService.sendReport
are used).
In
`@src/main/java/org/gridsuite/modification/server/service/NotificationService.java`:
- Around line 39-42: Add a dedicated, type-safe method
emitCaseResultMessage(CaseResultInfos payload) in NotificationService that
constructs a Message from the payload and calls the existing sendMessage(...)
with the correct binding name (following the pattern used by
emitBuildResultMessage and emitCancelBuildMessage); then restrict the generic
sendMessage(...) visibility to private (or package-private) so callers cannot
send to arbitrary bindings and the public API surface is limited to
purpose-specific emitXxx methods, referencing publisher.send(...) inside
sendMessage for actual dispatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 813a6807-03fd-47ba-8efd-3d07db803193
📒 Files selected for processing (9)
pom.xmlsrc/main/java/org/gridsuite/modification/server/NetworkModificationOnCaseController.javasrc/main/java/org/gridsuite/modification/server/service/CaseResultInfos.javasrc/main/java/org/gridsuite/modification/server/service/NetworkConversionService.javasrc/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.javasrc/main/java/org/gridsuite/modification/server/service/NotificationService.javasrc/main/resources/application-local.ymlsrc/main/resources/config/application.yamlsrc/main/resources/org/gridsuite/modification/server/reports.properties
Comment on lines
+110
to
+122
| private UUID save(Network network) throws IOException { | ||
| MemDataSource memDataSource = new MemDataSource(); | ||
| network.write(this.caseExportFormat, null, memDataSource); | ||
|
|
||
| Set<String> listNames = memDataSource.listNames(".*"); | ||
| String caseFileName = "apply-modifications-output." + this.caseExportFormat.toLowerCase(); | ||
| return save(new ByteArrayResource(memDataSource.getData(listNames.toArray()[0].toString())) { | ||
| @Override | ||
| public String getFilename() { | ||
| return caseFileName; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential ArrayIndexOutOfBoundsException if listNames is empty.
If memDataSource.listNames(".*") returns an empty set, accessing toArray()[0] will throw ArrayIndexOutOfBoundsException. Add a defensive check or use a safer access pattern.
🛡️ Suggested fix
private UUID save(Network network) throws IOException {
MemDataSource memDataSource = new MemDataSource();
network.write(this.caseExportFormat, null, memDataSource);
Set<String> listNames = memDataSource.listNames(".*");
+ if (listNames.isEmpty()) {
+ throw new PowsyblException("Network serialization produced no output files");
+ }
String caseFileName = "apply-modifications-output." + this.caseExportFormat.toLowerCase();
- return save(new ByteArrayResource(memDataSource.getData(listNames.toArray()[0].toString())) {
+ String outputName = listNames.iterator().next();
+ return save(new ByteArrayResource(memDataSource.getData(outputName)) {
`@Override`
public String getFilename() {
return caseFileName;
}
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private UUID save(Network network) throws IOException { | |
| MemDataSource memDataSource = new MemDataSource(); | |
| network.write(this.caseExportFormat, null, memDataSource); | |
| Set<String> listNames = memDataSource.listNames(".*"); | |
| String caseFileName = "apply-modifications-output." + this.caseExportFormat.toLowerCase(); | |
| return save(new ByteArrayResource(memDataSource.getData(listNames.toArray()[0].toString())) { | |
| @Override | |
| public String getFilename() { | |
| return caseFileName; | |
| } | |
| }); | |
| } | |
| private UUID save(Network network) throws IOException { | |
| MemDataSource memDataSource = new MemDataSource(); | |
| network.write(this.caseExportFormat, null, memDataSource); | |
| Set<String> listNames = memDataSource.listNames(".*"); | |
| if (listNames.isEmpty()) { | |
| throw new PowsyblException("Network serialization produced no output files"); | |
| } | |
| String caseFileName = "apply-modifications-output." + this.caseExportFormat.toLowerCase(); | |
| String outputName = listNames.iterator().next(); | |
| return save(new ByteArrayResource(memDataSource.getData(outputName)) { | |
| `@Override` | |
| public String getFilename() { | |
| return caseFileName; | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java`
around lines 110 - 122, In save(Network network) guard against
memDataSource.listNames(".*") returning empty: check if listNames.isEmpty() and
throw an IOException (or a specific checked exception) with a clear message
before using toArray()[0]; then replace the fragile toArray()[0] access with a
safe retrieval (e.g., listNames.iterator().next() or
listNames.stream().findFirst().get()) and continue creating the
ByteArrayResource with caseFileName and delegate to save(...). Ensure you
reference the memDataSource, listNames, caseExportFormat, ByteArrayResource and
the save(...) call when making the change.
Comment on lines
+162
to
+163
| } catch (Exception e) { | ||
| status = "FAILED"; |
There was a problem hiding this comment.
Critical: Exception is caught but silently discarded without logging.
The caught exception e is never logged or recorded, making it impossible to diagnose failures in production. At minimum, log the exception details.
🐛 Proposed fix
} catch (Exception e) {
+ LOGGER.error("Failed to apply modifications on case {}: {}", caseUuid, e.getMessage(), e);
status = "FAILED";
} finally {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (Exception e) { | |
| status = "FAILED"; | |
| } catch (Exception e) { | |
| LOGGER.error("Failed to apply modifications on case {}: {}", caseUuid, e.getMessage(), e); | |
| status = "FAILED"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/gridsuite/modification/server/service/NetworkModificationOnCaseService.java`
around lines 162 - 163, The catch block in NetworkModificationOnCaseService that
sets status = "FAILED" currently swallows the exception; update that catch to
log the exception (including stacktrace) using the class logger (e.g.,
logger.error("Failed to process network modification for case {}: {}",
caseIdOrContext, e.getMessage(), e)) so the error and context are recorded;
ensure you reference the existing logger field in
NetworkModificationOnCaseService and include any relevant identifiers (case
id/tenant id) available in the method to aid troubleshooting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
Apply modifications on a case